AI Review Summary

Verdict: REQUEST CHANGES
Commit: 7cc3b31

Findings

• [MEDIUM] src/cli/index.ts:122 – Potential race condition with async confirm in loop

  • The loop uses await inside a for...of loop to prompt for each file sequentially. While this works, if a user cancels mid-way through, the skillOverrides and workflowOverrides sets will be partially populated. This could lead to unexpected behavior where some files are marked for override but the installation is cancelled. Consider wrapping the entire loop in a try-catch or checking for cancellation before proceeding to installation.

  • Suggested fix:

    for (const file of existingFiles) {
      const shouldOverride = await p.confirm({
        message: `Override ${file.path}?`,
        initialValue: false,
      });
    
      if (p.isCancel(shouldOverride)) {
        p.cancel('Installation cancelled.');
        process.exit(0);
      }
    
      if (shouldOverride) {
        if (file.type === 'skill') {
          skillOverrides.add(file.name);
        } else if (file.type === 'workflow') {
          workflowOverrides.add(file.name);
        } else if (file.type === 'auth') {
          overrideAuth = true;
        }
      }
    }
    

• [MEDIUM] src/cli/index.ts:105 – checkExistingSkills called without filtering by selected workflows

  • When --workflows flag is used without --skills, the code still checks for ALL existing skills (line 105) even though skills won't be installed. This could prompt users to override skill files that won't actually be touched. The check should be skipped entirely when isWorkflowsOnly is true, which it already is on line 104, but the logic seems correct. However, when isSkillsOnly is true, line 109 checks workflows that won't be installed. This is inconsistent behavior.

  • Suggested fix:

    if (!isWorkflowsOnly) {
      existingFiles.push(...checkExistingSkills({}));
    }
    
    if (!isSkillsOnly) {
      existingFiles.push(...checkExistingWorkflows({ workflows: selectedWorkflows }));
      if (useOAuth) {
        const authFile = checkExistingAuthWorkflow({});
        if (authFile) {
          existingFiles.push(authFile);
        }
      }
    }
    

Overall Assessment

This PR adds a well-designed --force flag and interactive override prompts for existing files during installation. The implementation is solid with comprehensive test coverage. However, there are two medium-severity issues: a potential race condition in the interactive prompt loop and inconsistent handling of the --force flag when combined with --skills or --workflows options.